Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Couple fixes in rpmdb (double free, and rpmdbCheckTerminate return code) #92

Closed
wants to merge 2 commits into from
Closed

Couple fixes in rpmdb (double free, and rpmdbCheckTerminate return code) #92

wants to merge 2 commits into from

Conversation

glebfm
Copy link
Contributor

@glebfm glebfm commented Sep 19, 2016 •
edited by ignatenkobrain
Loading

This code:

#include <rpm/rpmdb.h>
#include <rpm/rpmts.h>
#include <rpm/rpmlib.h>
#include <signal.h>

class A {
    private:
        rpmts ts;
        rpmdbMatchIterator mi;
    public:
        A() {
            rpmReadConfigFiles(NULL, NULL);
            ts = rpmtsCreate();
            mi = rpmtsInitIterator(ts, RPMDBI_PACKAGES, NULL, 0);
        };
        ~A() {
            rpmdbFreeIterator(mi);
            rpmtsFree(ts);
        };
};

A a;

int main() {
    raise(SIGTERM);
    rpmdbCheckSignals();
    return 0;
}

tries to free MatchIterator again in atexit destructor.

Program received signal SIGSEGV, Segmentation fault.
#0  0x00007ffff7b57c17 in ?? () from /usr/lib64/librpm.so.7
#1  0x00007ffff7b5fbaa in rpmdbFreeIterator () from /usr/lib64/librpm.so.7
#2  0x00000000004009b6 in A::~A (this=0x601080 <a>, __in_chrg=<optimized out>) at t.cc:17
#3  0x00007ffff77d1ca8 in __run_exit_handlers (status=1, listp=0x7ffff7b395d8 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true) at exit.c:82
#4  0x00007ffff77d1cf5 in __GI_exit (status=<optimized out>) at exit.c:104
#5  0x00007ffff7b5f513 in rpmdbCheckSignals () from /usr/lib64/librpm.so.7
#6  0x00000000004008e9 in main () at t.cc:26

I tried if (rpmdbCheckTerminate(0) == 0) rpmdbFreeIterator(mi);, but rpmdbCheckTerminate return code is not reliable.

@glebfm
rpmdb.c: avoid double free in rpmdbClose, rpmdbMatchIterator, ...
9685b65 
... and rpmdbIndexIterator.
This makes functions assume that the object has been freed if it is not
on the list.

Signed-off-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
@glebfm
rpmdb.c: (rpmdbCheckTerminate) return non-zero on subsequent runs
94c0468 
This function is not necessarily called first by rpmdbCheckSignals, as
long as it is a part of API.  Thus, it is important to return the same
value on subsequent runs.

Signed-off-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
ignatenkobrain
ignatenkobrain approved these changes Sep 19, 2016
View reviewed changes
Copy link
Contributor

@ignatenkobrain ignatenkobrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, indeed!

Conan-Kudo
Conan-Kudo approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@Conan-Kudo Conan-Kudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me as well. 👍

@rpm-maint
Copy link

On 09/19/2016 07:33 PM, Gleb Pfotenhauer-Malinowski wrote:

This code:

#include <rpm/rpmdb.h>
#include <rpm/rpmts.h>
#include <rpm/rpmlib.h>
#include <signal.h>

class A {
    private:
        rpmts ts;
        rpmdbMatchIterator mi;
    public:
        A() {
            rpmReadConfigFiles(NULL, NULL);
            ts = rpmtsCreate();
            mi = rpmtsInitIterator(ts, RPMDBI_PACKAGES, NULL, 0);
        };
        ~A() {
            rpmdbFreeIterator(mi);
            rpmtsFree(ts);
        };
};

A a;

int main() {
    raise(SIGTERM);
    rpmdbCheckSignals();
    return 0;
}

tries to free MatchIterator again in atexit destructor.

Program received signal SIGSEGV, Segmentation fault.
#0 0x00007ffff7b57c17 in ?? () from /usr/lib64/librpm.so.7
#1 0x00007ffff7b5fbaa in rpmdbFreeIterator () from /usr/lib64/librpm.so.7
#2 0x00000000004009b6 in A::~A (this=0x601080 , __in_chrg=) at t.cc:17
#3 0x00007ffff77d1ca8 in __run_exit_handlers (status=1, listp=0x7ffff7b395d8 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true) at exit.c:82
#4 0x00007ffff77d1cf5 in __GI_exit (status=) at exit.c:104
#5 0x00007ffff7b5f513 in rpmdbCheckSignals () from /usr/lib64/librpm.so.7
#6 0x00000000004008e9 in main () at t.cc:26

I tried if (rpmdbCheckTerminate(0) == 0) rpmdbFreeIterator(mi);, but rpmdbCheckTerminate return code is not reliable.
You can view, comment on, or merge this pull request online at:

#92

-- Commit Summary --

  • rpmdb.c: avoid double free in rpmdbClose, rpmdbMatchIterator, ...
  • rpmdb.c: (rpmdbCheckTerminate) return non-zero on subsequent runs

-- File Changes --

M lib/rpmdb.c (17)

-- Patch Links --

https://github.com/rpm-software-management/rpm/pull/92.patch
https://github.com/rpm-software-management/rpm/pull/92.diff

Wow, that sounds like a fix for this rather ancient issue that I guess
nobody ever properly investigated:
https://www.redhat.com/archives/rpm-list/2002-November/msg00230.html

- Panu -

@ffesti
Copy link
Contributor

ffesti commented Sep 21, 2016

Nice catch! Always amazing what ancient bugs can still be found in even very basic code paths... sigh

Anyway. Thanks a lot for hunting this down. Pushed.

@ffesti ffesti closed this Sep 21, 2016
@glebfm glebfm deleted the rpmdb-fixes branch September 28, 2016 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ignatenkobrain ignatenkobrain ignatenkobrain approved these changes

@Conan-Kudo Conan-Kudo Conan-Kudo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@glebfm @rpm-maint @ffesti @Conan-Kudo @ignatenkobrain